Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IBX-6413: Search Suggestion (Autocomplete for search) #33

Merged
merged 66 commits into from
Nov 17, 2023
Merged

IBX-6413: Search Suggestion (Autocomplete for search) #33

merged 66 commits into from
Nov 17, 2023
+1,847 −26

Conversation

kisztof
Copy link
Contributor

@kisztof kisztof commented Oct 11, 2023 •
edited
Loading

Question Answer
Tickets IBX-6413
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes/no
Doc needed? yes
License GPL-2.0

Note

Related PR ibexa/admin-ui#959

Configuraton

  ibexa:
    system:
       default: # configuration per siteaccess or siteaccess group
           search:
               suggestion:
                   min_query_length: 3
                   result_limit: 5

Endpoint

Endpoint: /suggestion?query=ala&limit=10

###Endpoint structure

[
  {
    "contentId": 73,
    "contentTypeIdentifier": "article",
    "type": "content",
    "score": 50,
    "name": "Ala ma kota",
    "pathString": "2/74",
    "parentsLocation": [
      {
        "id": 52,
        "locationId": 2,
        "name": "Ibexa Digital Experience Platform"
      },
      {
        "id": 73,
        "locationId": 74,
        "name": "Ala ma kota"
      }
    ]
  },
  {
    "contentId": 74,
    "contentTypeIdentifier": "article",
    "type": "content",
    "score": 50,
    "name": "Ala ma psa",
    "pathString": "2/74/75",
    "parentsLocation": [
      {
        "id": 52,
        "locationId": 2,
        "name": "Ibexa Digital Experience Platform"
      },
      {
        "id": 73,
        "locationId": 74,
        "name": "Ala ma kota"
      },
      {
        "id": 74,
        "locationId": 75,
        "name": "Ala ma psa"
      }
    ]
  }
]

Implementing custom DataSource

\Ibexa\Contracts\Search\Event\SuggestionEvent

final class ContentSuggestionSubscriber implements EventSubscriberInterface
{
    public function __construct(CustomDataSource $customDataSource)
    {
        //...
    }
    public static function getSubscribedEvents(): array
    {
        return [
            SuggestionEvent::class => 'onSuggestion',
        ];
    }

    public function onSuggestion(SuggestionEvent $event): SuggestionEvent
    {
        $results = $this->customDataSource->find($event->getQuery);
        //...
        
        return $event;
    }
}

New suggestion model

The new suggestion model ought to inherit from \Ibexa\Contracts\Search\Model\Suggestion\Suggestion

final class CustomSuggestion extends Suggestion
{
}

Adding normalizers

Create your custom normalizer using implementation Symfony\Component\Serializer\Normalizer\NormalizerInterface

Annotate your custom normalizer using the tag ibexa.search.serializer.normalizer and it will be automatically used by the serializer.

    App\Normalizer\YourCustomNormalizer:
        tags:
              - { name: 'ibexa.search.serializer.normalizer' }

Checklist:

  • Implement tests
  • Coding standards ($ composer fix-cs)

@kisztof kisztof force-pushed the IBX-6413-part2-core-functionality branch from 55f32b1 to 885f626 Compare October 11, 2023 08:32
@kisztof kisztof force-pushed the IBX-6413-part2-core-functionality branch from 1705021 to d8fb688 Compare October 19, 2023 12:42
Steveb-p
Steveb-p reviewed Oct 23, 2023
View reviewed changes
tests/lib/Service/Event/SuggestionServiceTest.php Outdated Show resolved Hide resolved
src/lib/EventDispatcher/EventListener/ContentSuggestionSubscriber.php Outdated Show resolved Hide resolved
src/bundle/Controller/SuggestionController.php Outdated Show resolved Hide resolved
src/bundle/Resources/config/routing.yaml Outdated Show resolved Hide resolved
src/bundle/Controller/SuggestionController.php Outdated Show resolved Hide resolved
src/lib/Model/Suggestion/SuggestionCollection.php Outdated Show resolved Hide resolved
tests/lib/Service/Event/SuggestionServiceTest.php Outdated Show resolved Hide resolved
@kisztof kisztof marked this pull request as ready for review October 24, 2023 13:46
@kisztof kisztof changed the title [DRAFT] IBX-6413: part 2 - core functionality IBX-6413: Search Suggestion (Autocomplete for search) Oct 24, 2023
@kisztof kisztof requested a review from a team October 24, 2023 13:46
konradoboza
konradoboza reviewed Oct 24, 2023
View reviewed changes
src/bundle/DependencyInjection/Configuration/Parser/SiteAccessAware/SuggestionParser.php Outdated Show resolved Hide resolved
src/bundle/Resources/config/routing.yaml Outdated Show resolved Hide resolved
src/contracts/Mapper/SearchHitToContentSuggestionMapper.php Outdated Show resolved Hide resolved
src/contracts/Mapper/SearchHitToContentSuggestionMapper.php Outdated Show resolved Hide resolved
src/contracts/Provider/ParentLocationProvider.php Outdated Show resolved Hide resolved
src/lib/Mapper/SearchHitToContentSuggestionMapper.php Outdated Show resolved Hide resolved
src/lib/Model/Suggestion/SuggestionCollection.php Outdated Show resolved Hide resolved
src/lib/Serializer/SuggestionSerializer.php Outdated Show resolved Hide resolved
tests/lib/Service/Event/SuggestionServiceTest.php Outdated Show resolved Hide resolved
@konradoboza
Copy link

Also, SonarCloud seems to report some valid code smells.

@konradoboza konradoboza requested a review from a team October 24, 2023 14:02
Steveb-p
Steveb-p requested changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see an integration test for data serialization (encoding into JSON), because this is the crucial point and I think you might have a slight difference when there is 0, 1 and more items in the Location collection.

Steveb-p
Steveb-p approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integration tests, since those are missing on the package, can be postponed.

@bogusez bogusez self-assigned this Nov 14, 2023
This was referenced Nov 16, 2023
Closed
Closed
Closed
@bogusez
Copy link

bogusez commented Nov 17, 2023 •
edited
Loading

@kisztof as we discussed we have an error with the number of displayed autocomplete search results. The configuration that we define in ibexa.yaml doesn't respect the provided value. For example I set result_limit to 10 and autocomplete suggest me always 5 results even though, I have 6 contents in the system that meet the search conditions.

Edit:: I can approve the pull request and we can fix the issue right after beta3 release as you requested.

@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez
Copy link

bogusez commented Nov 17, 2023

@kisztof as we discussed we have an error with the number of displayed autocomplete search results. The configuration that we define in ibexa.yaml doesn't respect the provided value. For example I set result_limit to 10 and autocomplete suggest me always 5 results even though, I have 6 contents in the system that meet the search conditions.

Edit:: I can approve the pull request and we can fix the issue right after beta3 release as you requested.

bd48916
fix solved the issue. thanks

@bogusez
Copy link

bogusez commented Nov 17, 2023

Regression tests passed:
ibexa/commerce#476
ibexa/experience#272
ibexa/headless#28

@dew326 dew326 merged commit 32becdd into main Nov 17, 2023
10 checks passed
@dew326 dew326 deleted the IBX-6413-part2-core-functionality branch November 17, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriendupuis adriendupuis adriendupuis left review comments

@Steveb-p Steveb-p Steveb-p approved these changes

@bogusez bogusez bogusez approved these changes

@konradoboza konradoboza konradoboza approved these changes

@ViniTou ViniTou ViniTou approved these changes

@webhdx webhdx webhdx approved these changes

@alongosz alongosz Awaiting requested review from alongosz

@mikadamczyk mikadamczyk Awaiting requested review from mikadamczyk

Assignees

@kisztof kisztof

@ViniTou ViniTou

@bogusez bogusez

Labels
QA approved Ready for MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@kisztof @konradoboza @dew326 @bogusez @mikadamczyk @Steveb-p @alongosz @webhdx @ViniTou @adriendupuis